Skip to content

WASI MemoryFileSystem#230

Closed
andrewmd5 wants to merge 7 commits intoswiftwasm:mainfrom
6over3:main
Closed

WASI MemoryFileSystem#230
andrewmd5 wants to merge 7 commits intoswiftwasm:mainfrom
6over3:main

Conversation

@andrewmd5
Copy link
Copy Markdown
Contributor

Adds an in-memory file system to the WASI bridge (with the ability to mount actual file descriptors.)

@andrewmd5
Copy link
Copy Markdown
Contributor Author

CI failure seems unrelated to these changes (out of storage space,) but if they are let me know.

Comment thread Sources/WASI/FileSystem.swift Outdated
Comment thread Sources/WASI/FileSystem.swift Outdated
func getPreopenPaths() -> [String]

/// Opens a directory and returns a WASIDir implementation.
func openDirectory(at path: String) throws -> any WASIDir
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the use of existentials is justified here? Could we use opaque types instead?

Suggested change
func openDirectory(at path: String) throws -> any WASIDir
func openDirectory(at path: String) throws -> some WASIDir

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WASIDir seems to be defined as a protocol so I believe an existential is how its used elsewhere (i.e in Directory.swift)

Comment thread Sources/WASI/FileSystem.swift Outdated
Comment thread Sources/WASI/FileSystem.swift
Comment thread Sources/WASI/FileSystem.swift
import SystemPackage

/// Base protocol for all file system nodes in memory.
internal protocol MemFSNode: AnyObject {
Copy link
Copy Markdown
Member

@MaxDesiatov MaxDesiatov Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a strong reason to require AnyObject here? What would exactly classes (as opposed to non-copyable types) bring to the table other than reference counting performance hit?

Suggested change
internal protocol MemFSNode: AnyObject {
protocol MemFSNode {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple file entries need to share mutable access to the same node (e.g., a file opened twice should see the same content). With classes, this is natural, both entries hold references to the same object. With non-copyable structs, we'd need an indirection layer (handles/indices into a central store), which adds complexity. Open to that refactor if the ref-counting overhead is measurable or if I am mistaken (Swift is arguably very new to me.)

Comment thread Sources/WASI/MemoryFileSystem/MemoryFSNodes.swift Outdated
Comment thread Sources/WASI/MemoryFileSystem/MemoryFSNodes.swift Outdated
Comment thread Sources/WASI/MemoryFileSystem/MemoryFSNodes.swift Outdated
Comment thread Sources/WASI/MemoryFileSystem/MemoryFileEntry.swift Outdated
Comment thread Sources/WASI/FileSystem.swift Outdated
///
/// This implementation provides access to actual files and directories on the host system,
/// with appropriate sandboxing through pre-opened directories.
public final class HostFileSystem: FileSystemProvider, FileSystem {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usual concern WRT classes overhead here. IIUC we're not relying on class hierarchies, so why not make this a non-copyable struct to avoid the ARC overhead?

Suggested change
public final class HostFileSystem: FileSystemProvider, FileSystem {
public struct HostFileSystem: FileSystemProvider, FileSystemImplementation, ~Copyable {

Copy link
Copy Markdown
Contributor Author

@andrewmd5 andrewmd5 Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I went with class here is that FileSystemImplementation needs to be stored as a property in WASIBridgeToHost, and Swift's existential types (any Protocol) don't support ~Copyable types. If we make this a non-copyable struct, we'd need to make WASIBridgeToHost generic.

Copy link
Copy Markdown
Member

@MaxDesiatov MaxDesiatov Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kateinoigakukun do you have strong opinions about this? If these were non-public APIs I'd be ok in principle, but wouldn't be great to commit to classes and existentials here without providing high-performance alternatives for those who need it.

@MaxDesiatov
Copy link
Copy Markdown
Member

MaxDesiatov commented Jan 4, 2026

@andrewmd5 would you mind clarifying what's your use case here? I wonder if a cleaner way to do it is to focus on WASI 0.2+ and Component Model support in WasmKit instead. Then in-memory FS could be implemented in a separate component instead of WasmKit itself.

@andrewmd5
Copy link
Copy Markdown
Contributor Author

andrewmd5 commented Jan 4, 2026 via email

@MaxDesiatov
Copy link
Copy Markdown
Member

Given the lack of real support for WASI 0.2+ I don't think the component model is a realistic solution.

Would you elaborate? Lack of support in WasmKit or some other Wasm runtimes?

@andrewmd5
Copy link
Copy Markdown
Contributor Author

andrewmd5 commented Jan 4, 2026 via email

@kateinoigakukun
Copy link
Copy Markdown
Member

kateinoigakukun commented Jan 6, 2026

I agree with @andrewmd5's point regarding the reality of poor implementation status for Component Model in the ecosystem. So overall, it makes sense to me to add an in-memory FS implementation alongside the native implementation.

On the other hand, I think we need to refine the added APIs in this pull to be more Embedded Swift/performance-wise friendly before landing. At least we can avoid most of the existential container usage. If you don't mind, I can push my proposal to this branch. wdyt @andrewmd5 ?

@andrewmd5
Copy link
Copy Markdown
Contributor Author

andrewmd5 commented Jan 6, 2026 via email

@kateinoigakukun
Copy link
Copy Markdown
Member

@andrewmd5 It seems I don't have access to push your branch, so would you mind cherry-picking my change to your branch if you are happy with it? 23dc9f7

@kateinoigakukun
Copy link
Copy Markdown
Member

@andrewmd5 Just a gentle ping :) If you are busy, I can re-open a PR with my change on top of your branch and merge it to main.

@andrewmd5
Copy link
Copy Markdown
Contributor Author

@andrewmd5 Just a gentle ping :) If you are busy, I can re-open a PR with my change on top of your branch and merge it to main.

Let’s do that. I’m traveling currently and don’t want to block. Thanks!

kateinoigakukun added a commit that referenced this pull request Jan 13, 2026
Merge #230 into main

---------

Co-authored-by: andrewmd5 <1297077+andrewmd5@users.noreply.github.com>
@kateinoigakukun
Copy link
Copy Markdown
Member

kateinoigakukun commented Jan 13, 2026

Closing in favor of #259

Have a nice trip @andrewmd5!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants